Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refine error spans for "The trait bound T: Trait is not satisfied" when passing literal structs/tuples #106477

Merged

Conversation

Nathan-Fenner
Copy link
Contributor

@Nathan-Fenner Nathan-Fenner commented Jan 5, 2023

This PR adds a new heuristic which refines the error span reported for "T: Trait is not satisfied" errors, by "drilling down" into individual fields of structs/enums/tuples to point to the "problematic" value.

Here's a self-contained example of the difference in error span:

struct Burrito<Filling> {
    filling: Filling,
}
impl <Filling: Delicious> Delicious for Burrito<Filling> {}
fn eat_delicious_food<Food: Delicious>(food: Food) {}
fn will_type_error() {
    eat_delicious_food(Burrito { filling: Kale });
    //                 ^~~~~~~~~~~~~~~~~~~~~~~~~ (before) The trait bound `Kale: Delicious` is not satisfied
    //                                    ^~~~   (after)  The trait bound `Kale: Delicious` is not satisfied
}

(kale is fine, this is just a silly food-based example)

Before this PR, the error span is identified as the entire argument to the generic function eat_delicious_food. However, since only Kale is the "problematic" part, we can point at it specifically. In particular, the primary error message itself mentions the missing Kale: Delicious trait bound, so it's much clearer if this part is called out explicitly.


The existing heuristic tries to label the right function argument in point_at_arg_if_possible. It goes something like this:

  • Look at the broken base trait Food: Delicious and find which generics it mentions (in this case, only Food)
  • Look at the parameter type definitions and find which of them mention Filling (in this case, only food)
  • If there is exactly one relevant parameter, label the corresponding argument with the error span, instead of the entire call

This PR extends this heuristic by further refining the resulting expression span in the new point_at_specific_expr_if_possible function. For each impl in the (broken) chain, we apply the following strategy:

The strategy to determine this span involves connecting information about our generic impl
with information about our (struct) type and the (struct) literal expression:

  • Find the impl (impl <Filling: Delicious> Delicious for Burrito<Filling>)
    that links our obligation (Kale: Delicious) with the parent obligation (Burrito<Kale>: Delicious)
  • Find the "original" predicate constraint in the impl (Filling: Delicious) which produced our obligation.
  • Find all of the generics that are mentioned in the predicate (Filling).
  • Examine the Self type in the impl, and see which of its type argument(s) mention any of those generics.
  • Examing the definition for the Self type, and identify (for each of its variants) if there's a unique field
    which uses those generic arguments.
  • If there is a unique field mentioning the "blameable" arguments, use that field for the error span.

Before we do any of this logic, we recursively call point_at_specific_expr_if_possible on the parent
obligation. Hence we refine the expr "outwards-in" and bail at the first kind of expression/impl we don't recognize.

This function returns a Result<&Expr, &Expr> - either way, it returns the Expr whose span should be
reported as an error. If it is Ok, then it means it refined successfull. If it is Err, then it may be
only a partial success - but it cannot be refined even further.


I added a new test file which exercises this new behavior. A few existing tests were affected, since their error spans are now different. In one case, this leads to a different code suggestion for the autofix - although the new suggestion isn't wrong, it is different from what used to be.

This change doesn't create any new errors or remove any existing ones, it just adjusts the spans where they're presented.


Some considerations: right now, this check occurs in addition to some similar logic in adjust_fulfillment_error_for_expr_obligation function, which tidies up various kinds of error spans (not just trait-fulfillment error). It's possible that this new code would be better integrated into that function (or another one) - but I haven't looked into this yet.

Although this code only occurs when there's a type error, it's definitely not as efficient as possible. In particular, there are definitely some cases where it degrades to quadratic performance (e.g. for a trait impl with 100+ generic parameters or 100 levels deep nesting of generic types). I'm not sure if these are realistic enough to worry about optimizing yet.

There's also still a lot of repetition in some of the logic, where the behavior for different types (namely, struct vs enum variant) is similar but not the same.


I think the biggest win here is better targeting for tuples; in particular, if you're using tuples + traits to express variadic-like functions, the compiler can't tell you which part of a tuple has the wrong type, since the span will cover the entire argument. This change allows the individual field in the tuple to be highlighted, as in this example:

// NEW
LL |     want(Wrapper { value: (3, q) });
   |     ----                      ^ the trait `T3` is not implemented for `Q`

// OLD
LL |     want(Wrapper { value: (3, q) });
   |     ---- ^~~~~~~~~~~~~~~~~~~~~~~~~ the trait `T3` is not implemented for `Q`

Especially with large tuples, the existing error spans are not very effective at quickly narrowing down the source of the problem.

@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @compiler-errors (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 5, 2023
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left mostly a bunch of style nits. I've gotta take a few more passes over this code to reason about its correctness :)

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs Outdated Show resolved Hide resolved
};

// update the error span.
error.obligation.cause.span = expr
Copy link
Member

@compiler-errors compiler-errors Jan 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, so the span diverging from the actual hir id of the new obligation cause is very suspicious. It'll probably cause incorrect suggestions, since those can look at the type of the argument in the obligation cause via TypeckResults.

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs Outdated Show resolved Hide resolved
hir::ExprKind::Struct(
hir::QPath::Resolved(
_,
hir::Path { res: hir::def::Res::Def(struct_def_kind, struct_def_id), .. },
Copy link
Member

@compiler-errors compiler-errors Jan 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a way of extracting the def here rather than destructuring the path's res -- try type_dependent_def

This will help us in cases where the path is resolved during type-check rather than during the earlier name resolve stage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used .qpath_res here instead; type_dependent_def doesn't seem to work, I think because this isn't a call?

There's a second use of pretty much the same logic below, where type_dependent_def might work. I haven't tried that yet.

Copy link
Member

@compiler-errors compiler-errors Jan 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad -- qpath_res is the one I was thinking of, yeah. That'll store the properly resolved res for a qpath, regardless of it's TypeRelative or Resolved

Comment on lines 2191 to 2194
let struct_def_generics: Vec<&ty::GenericParamDef> = relevant_ty_args_indices
.into_iter()
.filter_map(|index| struct_def_generics.params.get(index))
.collect();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this, and similar below, could be a retain :laugh:

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

Some considerations: right now, this check occurs in parallel to the independent adjust_fulfillment_error_for_expr_obligation function.

I'm not sure if I understand this comment, point_at_specific_expr_if_possible is being called in point_at_arg_if_possible, which happens within adjust_fulfillment_error_for_expr_obligation. Is that comment out of date, or am I misunderstanding what it's saying?

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more mostly nit-picks.

Needless to say, I'm quite happy with the state of this PR as-is, so please don't take my comments in a negative light 😃

compiler/rustc_middle/src/traits/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/select/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs Outdated Show resolved Hide resolved
@Nathan-Fenner
Copy link
Contributor Author

I've fixed up most of the small nits, and marked those comments with (👍) - I'm not sure for rustc if the policy is to prefer the reviewer resolve their own comments, or the PR author?

I'll try to get to the rest of the nits tomorrow. They all seem reasonable, it will just take me a bit to fix some of them.

@compiler-errors
Copy link
Member

Feel free to click resolve as you fix things. I'll resolve the ones I see are outdated when I do another review of this tomorrow. And sure, take your time addressing feedback, and as always, speak up if you disagree with my suggestions.

@Nathan-Fenner
Copy link
Contributor Author

I'm not sure if I understand this comment, point_at_specific_expr_if_possible is being called in point_at_arg_if_possible, which happens within adjust_fulfillment_error_for_expr_obligation. Is that comment out of date, or am I misunderstanding what it's saying?

I meant that adjust_fulfillment_error_for_expr_obligation is doing a bunch of similar span-reduction logic, of which point_at_arg_if_possible is just one small part. It might make more sense to move this logic into adjust_fulfillment_error_for_expr_obligation directly, or as a helper that it directly calls, so that it can "work more closely" with its existing span reductions. For example, it's not clear to me if it would ever be helpful to do adjust_fulfillment_error_for_expr_obligation, go through the new point_at_specific_expr_if_possible, and then adjust again using the existing heuristics in adjust_fulfillment_error_for_expr_obligation- or if it's always best to do them "outside-in" like I picked here.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

return true;
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old way of doing things and the new way of doing things introduced in this PR are in conflict. The old way mutate the span in place, and returns true if it does this.

The new way uses Result<Expr, Expr> to indicate whether it successfully refined or unsuccessfully refined. But it can still make a partial refinement in the unsuccessful case. The downside is that it needs to know the "root" expr to return it in failure.


I think a hybrid approach is actually going to be nicer. I will introduce a new ErrorSpanRefiner which can be called with:

struct FullyRefined;
struct NotFullyRefined;
fn refine_span_to_expr(&mut self, expr: &hir::Expr<'tcx>) -> Result<(), NotFullyRefined>

and the new/existing point_to_*_ functions will be updated to return Result<FullyRefined, NotFullyRefined> instead of bool or Result<&Expr, &Expr>.

The ErrorSpanRefiner can also deal with updating the cause, as needed, so that the cause and the span stay in sync.

@rust-log-analyzer

This comment has been minimized.

@@ -1863,23 +1863,26 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
return true;
}
}
hir::ExprKind::Struct(qpath, fields, ..) => {
if let Res::Def(DefKind::Struct | DefKind::Variant, variant_def_id) =
hir::ExprKind::Struct(qpath, _fields, ..) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing you can do after the rest of the review is done -- all of these methods under adjust_fulfillment_errors_for_expr_obligation can be moved into a distinct module. Maybe adjust_fulfillment_errors.rs or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into some merge conflicts trying to move the existing functions - the new ones are currently placed into a separate module. I'd like to move the remaining ones in a separate PR, just to avoid rebase-churn on this one, since it adds a lot of net-new code.

@Nathan-Fenner Nathan-Fenner force-pushed the nathanf/refined-error-span-trait-impl branch from a2b69cf to 56e2593 Compare January 9, 2023 06:45
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jan 9, 2023

☔ The latest upstream changes (presumably #106637) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors
Copy link
Member

compiler-errors commented Jan 17, 2023

Howdy -- sorry for not reviewing this in the mean time. This PR needs to be rebased and tests need to be fixed/blessed. Can you do that @Nathan-Fenner? (no rush obviously)

@compiler-errors
Copy link
Member

(use @rustbot ready to mark this ready for review once it's rebased)

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 18, 2023
@Nathan-Fenner Nathan-Fenner force-pushed the nathanf/refined-error-span-trait-impl branch from 56e2593 to aa30dd7 Compare January 23, 2023 19:33
@rustbot
Copy link
Collaborator

rustbot commented Jan 23, 2023

Some changes occurred in src/tools/cargo

cc @ehuss

@Nathan-Fenner Nathan-Fenner force-pushed the nathanf/refined-error-span-trait-impl branch from aa30dd7 to bd8812b Compare January 23, 2023 19:55
@rust-log-analyzer

This comment has been minimized.

@Nathan-Fenner Nathan-Fenner force-pushed the nathanf/refined-error-span-trait-impl branch from bd8812b to 2a67e99 Compare January 23, 2023 21:38
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

Subtle reminder for the future to use

@rustbot ready

to mark this as ready for review. Sorry, hadn't seen that this was updated because it was marked as S-waiting-on-author. I'll take a look soon.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 3, 2023
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'm pretty happy with the changes to the logic.

Err(expr)
}

// FIXME: This can be made into a private, non-impl function later.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should follow-up with these FIXMEs. If you can't in the next few weeks, then I will.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 6, 2023

📌 Commit 99638a6 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 6, 2023
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#106477 (Refine error spans for "The trait bound `T: Trait` is not satisfied" when passing literal structs/tuples)
 - rust-lang#107596 (Add nicer output to PGO build timer)
 - rust-lang#107692 (Sort Generator `print-type-sizes` according to their yield points)
 - rust-lang#107714 (Clarify wording on f64::round() and f32::round())
 - rust-lang#107720 (end entry paragraph with a period (.))
 - rust-lang#107724 (remove unused rustc_* imports)
 - rust-lang#107725 (Turn MarkdownWithToc into a struct with named fields)
 - rust-lang#107731 (interpret: move discriminant reading and writing to separate file)
 - rust-lang#107735 (Add mailmap for commits made by [email protected])

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 800221b into rust-lang:master Feb 6, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 6, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 7, 2023
…ust_fulfillment_errors, r=compiler-errors

Split fn_ctxt/adjust_fulfillment_errors from fn_ctxt/checks

This is a follow-up from rust-lang#106477, addressing a small number of the `FIXME`s that were added, by moving some functions into the new(er) `adjust_fulfillment_errors` module.

More cleanup is possible for this file (and I'll hopefully get around to doing some of that soon) but the very first thing is to just move these functions out.

There should be no "real" changes in this PR, besides minor adjustments to imports and the functions being transferred.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 20, 2023
…ef-trait-refine, r=WaffleLapkin

Refine error span for trait error into borrowed expression

Extends the error span refinement in rust-lang#106477 to drill into borrowed expressions just like tuples/struct/enum literals. For example,

```rs
trait Fancy {}
trait Good {}
impl <'a, T> Fancy for &'a T where T: Good {}
impl <S> Good for Option<S> where S: Iterator {}

fn want_fancy<F>(f: F) where F: Fancy {}

fn example() {
    want_fancy(&Some(5));
//  (BEFORE)   ^^^^^^^^ `{integer}` is not an iterator
//  (AFTER)          ^  `{integer}` is not an iterator
}
```

Existing heuristics try to find the right part of the expression to "point at"; current heuristics look at e.g. struct constructors and tuples. This PR adds a new check for borrowed expressions when looking into a borrowed type.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Mar 2, 2023
…-span-fix-Some, r=WaffleLapkin

Point error span at Some constructor argument when trait resolution fails

This is a follow up to rust-lang#108254 and rust-lang#106477 which extends error span refinement to handle a case which I mistakenly believed was handled in rust-lang#106477. The goal is to refine the error span depicted below:

```rs
trait Fancy {}
impl <T> Fancy for Option<T> where T: Iterator {}

fn want_fancy<F>(f: F) where F: Fancy {}

fn example() {
    want_fancy(Some(5));
//  (BEFORE)   ^^^^^^^ `{integer}` is not an iterator
//  (AFTER)         ^  `{integer}` is not an iterator
}
```

I had used a (slightly more complex) example as an illustrative example in rust-lang#108254 , but hadn't actually turned it into a test, because I had (incorrectly) believed at the time it was covered by existing behavior. It turns out that `Some` is slightly "special" in that it resolves differently from the other `enum` constructors I had tried, and therefore this test was actually broken.

I've now updated the tests to include this example, and fixed the code to correctly resolve the `Some` constructor so that the span of the error is reduced.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 2, 2023
…-span-fix-Some, r=WaffleLapkin

Point error span at Some constructor argument when trait resolution fails

This is a follow up to rust-lang#108254 and rust-lang#106477 which extends error span refinement to handle a case which I mistakenly believed was handled in rust-lang#106477. The goal is to refine the error span depicted below:

```rs
trait Fancy {}
impl <T> Fancy for Option<T> where T: Iterator {}

fn want_fancy<F>(f: F) where F: Fancy {}

fn example() {
    want_fancy(Some(5));
//  (BEFORE)   ^^^^^^^ `{integer}` is not an iterator
//  (AFTER)         ^  `{integer}` is not an iterator
}
```

I had used a (slightly more complex) example as an illustrative example in rust-lang#108254 , but hadn't actually turned it into a test, because I had (incorrectly) believed at the time it was covered by existing behavior. It turns out that `Some` is slightly "special" in that it resolves differently from the other `enum` constructors I had tried, and therefore this test was actually broken.

I've now updated the tests to include this example, and fixed the code to correctly resolve the `Some` constructor so that the span of the error is reduced.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 2, 2023
…-span-fix-Some, r=WaffleLapkin

Point error span at Some constructor argument when trait resolution fails

This is a follow up to rust-lang#108254 and rust-lang#106477 which extends error span refinement to handle a case which I mistakenly believed was handled in rust-lang#106477. The goal is to refine the error span depicted below:

```rs
trait Fancy {}
impl <T> Fancy for Option<T> where T: Iterator {}

fn want_fancy<F>(f: F) where F: Fancy {}

fn example() {
    want_fancy(Some(5));
//  (BEFORE)   ^^^^^^^ `{integer}` is not an iterator
//  (AFTER)         ^  `{integer}` is not an iterator
}
```

I had used a (slightly more complex) example as an illustrative example in rust-lang#108254 , but hadn't actually turned it into a test, because I had (incorrectly) believed at the time it was covered by existing behavior. It turns out that `Some` is slightly "special" in that it resolves differently from the other `enum` constructors I had tried, and therefore this test was actually broken.

I've now updated the tests to include this example, and fixed the code to correctly resolve the `Some` constructor so that the span of the error is reduced.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants